Skip to content

Support forwarded https#22492

Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom
wills-feng:Support-forwarded-https
Apr 24, 2024
Merged

Support forwarded https#22492
tdcmeehan merged 1 commit intoprestodb:masterfrom
wills-feng:Support-forwarded-https

Conversation

@wills-feng
Copy link
Contributor

@wills-feng wills-feng commented Apr 11, 2024 •
edited
Loading

Description

Currently Presto will skip authentication when request is inSecure(http), but in a common situation that presto is deployed behind a load balancer which will handle SSL termination, we expect presto can recognize X-Forwarded-Proto header and continue with configured authentication.
This feature was added in Ahana presto by this PR.

Motivation and Context

Support authentication for Presto behind a proxy

Impact

The flag is turned off by default.

Test Plan

Deploy presto with password authentication behind nginx, authentication method applied.

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Add `http-server.authentication.allow-forwarded-https` configuration property to recognize X-Forwarded-Proto header, :pr:`22492`

@wills-feng wills-feng requested a review from a team as a code owner April 11, 2024 19:23
@wills-feng wills-feng requested a review from presto-oss April 11, 2024 19:23
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 11, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: wills-feng / name: WillsFeng (9f36555)

@wills-feng
Copy link
Contributor Author

wills-feng commented Apr 11, 2024

@yzhang1991 @wanglinsong @tdcmeehan

@steveburnett
Copy link
Contributor

steveburnett commented Apr 11, 2024

  • If adding a user-facing configuration property, maybe a release note entry similar to the following is appropriate?
== RELEASE NOTES ==

General Changes
* Add setAllowForwardedHttps configuration property to recognize X-Forwarded-Proto header, :pr:`22492`

@wills-feng wills-feng marked this pull request as draft April 11, 2024 20:25
@wills-feng wills-feng force-pushed the Support-forwarded-https branch 2 times, most recently from 5b34da0 to e131a43 Compare April 12, 2024 00:20
@wills-feng
Copy link
Contributor Author

wills-feng commented Apr 12, 2024

Fixed the CLA and update the docs

@wills-feng wills-feng marked this pull request as ready for review April 12, 2024 00:24
@wills-feng wills-feng changed the title [WIP]Support forwarded https Support forwarded https Apr 12, 2024
@github-actions
Copy link

github-actions bot commented Apr 12, 2024 •
edited
Loading

Codenotify: Notifying subscribers in CODENOTIFY files for diff d2b2ae9...9f36555.

Notify File(s)
@steveburnett presto-docs/src/main/sphinx/security/ldap.rst

steveburnett
steveburnett requested changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the doc! Found a few minor nits of formatting for consistency with the rest of the documentation, but looks good!

@tdcmeehan tdcmeehan self-assigned this Apr 17, 2024
@tdcmeehan
Copy link
Contributor

tdcmeehan commented Apr 17, 2024

LGTM but please incorporate suggestions from @steveburnett

@wills-feng wills-feng force-pushed the Support-forwarded-https branch from e131a43 to 9f36555 Compare April 19, 2024 16:06
steveburnett
steveburnett approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (docs)

Pull updated branch, new local build, everything looks good.

@wills-feng
Copy link
Contributor Author

wills-feng commented Apr 23, 2024

Thanks @steveburnett. @wanglinsong just confirmed the change works in our env. @tdcmeehan Could you please approval and merge?

wanglinsong
wanglinsong approved these changes Apr 23, 2024
View reviewed changes
Copy link
Member

@wanglinsong wanglinsong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have deployed the updates and verified the password requirement is enabled.

@tdcmeehan tdcmeehan merged commit 369f9d3 into prestodb:master Apr 24, 2024
@wills-feng wills-feng deleted the Support-forwarded-https branch April 24, 2024 18:43
@wanglinsong wanglinsong mentioned this pull request Jun 25, 2024
Merged
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@steveburnett steveburnett steveburnett approved these changes

@tdcmeehan tdcmeehan tdcmeehan approved these changes

@wanglinsong wanglinsong wanglinsong approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

Assignees

@tdcmeehan tdcmeehan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wills-feng @steveburnett @tdcmeehan @wanglinsong